Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Proposal for the future DX of the Date and Time Pickers #15613

Draft
wants to merge 61 commits into
base: master
Choose a base branch
from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 26, 2024

Link to the RFC (all the pages are in the expanded page group on the left nav bar).

Preliminary notes

  • Not everything described in this PR has to be ready for v8 or even v9, priorisation can come once we know what we want to build
  • This document is here to gather feedbacks, the DX is still very much WIP, if you don't like something, don't hesitate to say why
  • Some props are missing and are implemented in [POC][pickers] Explore the Base UI Calendar component #16069

Which components?

  • Calendar
  • RangeCalendar
  • PickerField
  • Picker (+ maybe a RangePicker)
  • DigitalClock

With those 5 / 6 components, we would cover almost all the UI of the date and time pickers.

@flaviendelangle flaviendelangle self-assigned this Nov 26, 2024
@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Nov 26, 2024
@flaviendelangle flaviendelangle force-pushed the rfc-base-ui branch 13 times, most recently from 0e16795 to 7060489 Compare November 27, 2024 08:31
@flaviendelangle flaviendelangle force-pushed the rfc-base-ui branch 4 times, most recently from 2d68e94 to 0470967 Compare November 27, 2024 14:05
@flaviendelangle flaviendelangle changed the title [RFC] WIP - Proposal for the future DX of the Date and Time Pickers [RFC] Proposal for the future DX of the Date and Time Pickers Jan 20, 2025
Copy link
Member

@KenanYusuf KenanYusuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't had a chance to read through all of it yet, but it's looking great.

Comment on lines +27 to +29
<RangeCalendar.GoToMonth target="previous">◀</RangeCalendar.GoToMonth>
{visibleMonth.format('MMMM YYYY')}
<RangeCalendar.GoToMonth target="next">▶</RangeCalendar.GoToMonth>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming suggestion:

Suggested change
<RangeCalendar.GoToMonth target="previous">◀</RangeCalendar.GoToMonth>
{visibleMonth.format('MMMM YYYY')}
<RangeCalendar.GoToMonth target="next">▶</RangeCalendar.GoToMonth>
<RangeCalendar.MonthNavigation direction="previous">◀</RangeCalendar.GoToMonth>
{visibleMonth.format('MMMM YYYY')}
<RangeCalendar.MonthNavigation direction="next">▶</RangeCalendar.GoToMonth>

Copy link
Member Author

@flaviendelangle flaviendelangle Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the name of the component, on the POC I'm currently using Calendar.SetVisibleMonth.
The idea was that it's consistent with the state it updates (which is named visibleDate).
Calendar.MonthNavigation also works well, maybe we can wait to see other similar button components on our advanced components and then try to have a coherent naming strategy (with / without a verb)

For the target / direction prop, I think direction is problematic now that we can also pass a date directly to the component and not only "next" / "previous".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sort of on the same page about the naming here. Feels a bit off to name a component like you would name a function 🤔 I'm all in for keeping the visibleMonth/Date/Year in the name, but maybe visibleMonthNavigation (or so9mething in that area 😅 ) could work better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most important is to be consistent with the other Base UI / Base UI X primitives.
I see that Base UI uses <Menu.Trigger /> and I guess the big question is, if you have two triggers, would you name it <MenuTriggerA /> (trigger is a verb) or <Menu.ATrigger /> (trigger is a noun).

I'm totally fine with both.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we have two triggers (or two very similar actions) for the exact same component?

I could imagine having something like SomeComponent.Trigger and OtherComponent.Trigger but I might very well be missing stuff

But in the scenario you are describing, I would lean towards <Menu.ATrigger />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was more a way to say that the current nomenclature <Menu.Trigger /> on Base UI can be understood both as a noun and as a verb.
So when we try to create buttons with more detailed names, some of us might prefer to treat it as a noun or as a verb.

<React.Fragment>
<header>
<Calendar.SetVisibleMonth target="previous">◀</Calendar.SetVisibleMonth>
{visibleMonth.format('MMMM YYYY')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{visibleMonth.format('MMMM YYYY')}
<Calendar.MonthLabel format="MMMM YYYY" />

Wonder if this should be wrapped in a component as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a small typo here, it should be visibleDate not visibleMonth that is exposed.

On the 1st version, I had a <Calendar.FormattedValue /> component that was taking a format prop and formatting the visible date.
The problem was that on some example I wanted to use the visibleDate, on others I wanted to use the selected value. So I needed either two components, or one component with a prop to say which state was targetted.

And when you render two months, you may want to show the two month name in the header, so maybe also an "offset" prop.

It was getting quite complex for the very little shortcut it gives.
Which is why I went for not providing a component, but instead rely on direct state access, at least until we have more value to give to this component.

Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good 🔥 I'm leaving a few nitpicks about naming. I didn't dive deep into everything - I will go through the other pages in the following days 🙈


### Without Material UI

The user can use the `<PickerField.Clear />` component to add a button to clear the value:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the naming of this. Maybe:

Suggested change
The user can use the `<PickerField.Clear />` component to add a button to clear the value:
The user can use the `<PickerField.ClearButton />` component to add a button to clear the value:

It's a nitpick, and every component that triggers some sort of action should probably have a consistent naming, so feel free to ignore my comment if it conflicts with what you have planned for other parts of the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Base UI team used <Menu.Trigger /> not <Menu.TriggerButton />, same for <Menu.Increment />.
So I don't think adding "Button" would fit nicely.

But maybe <PickerFIeld.Clear /> is too vague and short and <PickerField.ClearValue /> would be better (or something else).
Clearly the naming of the buttons seems to be one of the hot spot of the new DX 😆

The new DX is a good opportunity to discuss the behavior of this prop.
The behavior should either be:

1. `onClear` is defined on `<PickerField.Root />`. It is also fired when doing <kbd class="key">Ctrl</kbd> + <kbd class="key">A</kbd> and then <kbd class="key">Backspace</kbd>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having one onClear method defined on the higher level, that is executed every time a clearing action is triggered (click on the button or by keyboard). Makes things more consistent IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also what I would prefer thinking about it right now.

<Calendar.DaysGridBody>
{({ weeks }) =>
weeks.map((week) => (
<Calendar.DaysWeekRow value={week}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it slightly confusing that The prefix of most of the component names here is DaysGrid but this one is called DaysWeekRow. We could consider renaming it to DaysGridRow for consistency?

Copy link
Member Author

@flaviendelangle flaviendelangle Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting...
I went for DaysWeekRow because I wanted to highlight that 1 row = 1 week.
But maybe it's so obvious that it's not worth the slight inconsistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe it's so obvious that it's not worth the slight inconsistency.

I think it's clear that week == row. And I'm not sure we would have anything that we could introduce to make it unclear (some other component that would be called row for instance). So I'm all for consistency in this case 👌

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update both the RFC and the POC tomorrow to see if somebody thinks otherwise 😆

And related question, would you name the params in the render function weeks and days or rows and cells (in which case the month and year render functions would have cells instead of months and years)

I went back and forth on this one.
On the time view I chose option on the HourOptions and equivalent primitives, because if I named it with the name of the section, the MeridiemOptions was receiving meridies if you follow the latin plural and thats far from obvious 😆 , but I could not find a better alternative.
Some maybe cells / rows is better, and options for the time since it's also the name of the component (or cells if I rename the Option component into Cell and we have something consistent everywhere).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm...interesting question 🤔 I don't have a very very strong preference there.
For me cells/rows is more something that has to do with the layout. So Calendar.DaysGridRow and Calendar.DaysCell make sense as something that clearly helps you build your layout. But the render function params indicate what you are displaying inside the layout so maybe keeping weeks, days, months and years is somewhat justified too 🤔 Not sure if that makes sense

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make sense
I think it's super subjective, and if you asked a lot of people I wouldn't be surprised if we ended up with a 40/60 answer ratio

Comment on lines +27 to +29
<RangeCalendar.GoToMonth target="previous">◀</RangeCalendar.GoToMonth>
{visibleMonth.format('MMMM YYYY')}
<RangeCalendar.GoToMonth target="next">▶</RangeCalendar.GoToMonth>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sort of on the same page about the naming here. Feels a bit off to name a component like you would name a function 🤔 I'm all in for keeping the visibleMonth/Date/Year in the name, but maybe visibleMonthNavigation (or so9mething in that area 😅 ) could work better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants